Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Suspend Broken Tenants & Delete Broken Clients #177

Merged
merged 5 commits into from
Aug 9, 2023

Conversation

HarryET
Copy link
Contributor

@HarryET HarryET commented Aug 8, 2023

Description

Saves IP reputation and removes random 5XX errors

How Has This Been Tested?

N/a

Due Diligence

  • Breaking change
  • Requires a documentation update
  • Requires a e2e/integration test update

@HarryET HarryET self-assigned this Aug 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

No linked issues found. Please add the corresponding issues in the pull request description.
Use GitHub automation to close the issue when a PR is merged

Copy link
Member

@arein arein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM other than tests are missing here as well.
Would be good to cover in functional tests

Ok(_) => Ok(()),
Err(error) => match error {
Error::BadDeviceToken => {
state.client_store.delete_client(&tenant_id, &id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this a soft delete?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No its a hard delete as the data we have stored is now useless, we still have data about the client in datalake

src/stores/tenant.rs Outdated Show resolved Hide resolved
src/providers/fcm.rs Outdated Show resolved Hide resolved
src/providers/fcm.rs Outdated Show resolved Hide resolved
@@ -435,4 +452,8 @@ impl TenantStore for DefaultTenantStore {
) -> Result<Tenant> {
panic!("Shouldn't have run in single tenant mode")
}

async fn suspend_tenant(&self, id: &str, reason: &str) -> Result<()> {
panic!("Shouldn't have run in single tenant mode")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error message could be improved:

Suggested change
panic!("Shouldn't have run in single tenant mode")
panic!("Invalid APNS or FCM credentials. Cannot suspend tenant in single-tenant mode, so panicking instead.")

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There error message is the same for all the functions, they cannot be run in single-tenant mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the other functions' error messages could be improved too?

I just don't see any debugging info that would help a service maintainer know that their credential had expired.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdym? The panics here are for function calls that won't/can't be done in single-tenant mode

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, if they are impossible to call, then you should use unreachable!() instead of panic!()

src/error.rs Outdated Show resolved Hide resolved
src/error.rs Outdated Show resolved Hide resolved
migrations/1691518766_add-suspension.sql Show resolved Hide resolved
@HarryET HarryET merged commit a8e1aa7 into main Aug 9, 2023
3 of 4 checks passed
@HarryET HarryET deleted the feat/tenant-suspension branch August 9, 2023 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants